-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite devices dashboard page in react #6489
base: master
Are you sure you want to change the base?
Conversation
Cloudflare Pages deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint doesn't pass. Please fix all ESLint issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint doesn't pass. Please fix all ESLint issues.
accessorFn: row => parseISO8601Date(row.DateLastActivity), | ||
header: globalize.translate('LabelTime'), | ||
size: 160, | ||
Cell: ({ cell }) => toLocaleString(cell.getValue<Date>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would toLocaleDateString
with getDisplayTime
be more readable? (example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just use standard approaches for displaying the date time... I did switch to using date-fns
though since eventually we should replace the custom date formatting stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a personal suggestion, but we could consider removing the seconds part in favor of easier comprehension at a glance.
...DEFAULT_TABLE_OPTIONS, | ||
|
||
columns, | ||
data: devices?.Items || [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you are tabbed out for a while and come back to the page or when deleting/editing a device, react query will re-render the data causing the table state (current page, filters, etc) to reset.
We might be able to fix this by adding autoResetPageIndex: isRefetching
(have not tried).
This likely affects other tables as well.
// Enable custom features | ||
enableColumnPinning: true, | ||
enableColumnResizing: true, | ||
|
||
// Sticky header/footer | ||
enableStickyFooter: true, | ||
enableStickyHeader: true, | ||
muiTableContainerProps: { | ||
sx: { | ||
maxHeight: 'calc(100% - 7rem)' // 2 x 3.5rem for header and footer | ||
} | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this refactor for the api keys page as well? 😅
I'm also alright with doing it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It didn't exist when I started this and I didn't want to make the PR bigger. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint doesn't pass. Please fix all ESLint issues.
Quality Gate passedIssues Measures |
Changes
Rewrites the devices dashboard page in react using the table pattern
Issues
N/A